fix day range parsing (zk-org/zk#382) #384
Conversation
|
Could we get a test for this fix, so we don't see a regression? Like the following: but in this file: https://github.com/zk-org/zk/blob/main/internal/cli/filtering_test.goIt would need to fail before your changes, and succeed with the' |
|
yesss. good idea 😅 Althought, it should actually be tested with the The go tests in filtering_test.go aren't operating on any actual example data from what I see. In other words, 'today' will always pass if the function executes and returns successfully. So by that token, the current tesh tests should already be covering this change as in But, perhaps there aren't any test notes that have a time stamp of 00:00:00, which was the edge case, so will investigate that anyway in the meantime. |
| start = startOfDay(day) | ||
| // we add -1 second so that the day range ends at 23:59:59 | ||
| // i.e, the 'new day' begins at 00:00:00 | ||
| start = startOfDay(day).Add(time.Second * -1) |
There was a problem hiding this comment.
What was the value of start before? 00:00:00 or 00:00:01 ?
If start was already 00:00:00 then it seems that subtracting a second should be done to from end.
There was a problem hiding this comment.
Logically that does make sense in how I would also think a day range should be. But the issue is that when a note is created without a time stamp in YAML, that it's given the time 00:00. But it would seem the time library considers 00:00 to be midnight of the upcoming change over to the next day.
So if the second was subtracted from the end, and you made a note without a time stamp today, it would appear in a query for notes being created tomorrow (even though I don't think that query exists? It's just to illustrate the point)
In any case, subtracing 1 second from the end of the day, causes the issue-382.tesh test to fail. That test is a note created date: 2024-01-24 (without a time stamp), and queried with zk list -q --created 2024-01-24.
Is there another way to work around this that keeps a more logical way of interpreting a day range?
fix: set day range end to be one second before midnight.
i.e, the next day begins at 00:00.
Now the day range extends to 23:59:59. Meaning that the notes created within the
same day, that do not have time stamps, will be returned by a query such as
zk list --created=today, previously would fall under... --created=yesterday.Likewise, given a note with the YAML date
2024-01-22, zk would report this as beingcreated on
2024-01-21.